Skip to content

Conversation

@llrs-roche
Copy link
Collaborator

@llrs-roche llrs-roche commented Dec 11, 2025

Incomplete PR to fix some issues with the package. It aims to:

  • fix some notes.
  • Address the example failing

Fixes: #1

@llrs-roche llrs-roche changed the title Fixes fo Fixes package Dec 11, 2025
@llrs-roche llrs-roche marked this pull request as ready for review January 15, 2026 11:22
@dominik-appsilon dominik-appsilon self-requested a review January 15, 2026 15:11
@llrs-roche llrs-roche linked an issue Jan 15, 2026 that may be closed by this pull request
Copy link

@dominik-appsilon dominik-appsilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the changes! Almost everything looks fine, I have just one major doubt about logic of checking paths in package_report() -- see the comment.

In general, I fell like operations on files in R/reporter.R would use some refactoring and overall cleanup. One of the particular issues I had was that due to missing dependencies report generation failed and temporary files got generated but not removed and this caused subsequent calls to package_report() to fail as well (multiple .qmd files). However, this is rather not something that should be worked on in this particular PR, just something to note for the future.

During testing I found one additional issue with test intests/testthat/test-render_table.R. It reads assessment from {riskreport} which, if not installed, fails. I assume it should be changed to {val.report}. But while at it -- do we use this .rds file only for testing? Do we want to store it with package? We could also mock it instead.

Copy link
Collaborator Author

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments and review! Let's open a new issue for the clean up and file manipulation. I too feel that they should be simpler but didn't want to modify them too much here.

Copy link

@dominik-appsilon dominik-appsilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and all the explanations! I think it's good to go!

@llrs-roche llrs-roche merged commit 8b30d51 into main Jan 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check is failing Rendering report results in an error Date is always set to 12:00:00

3 participants